Conversation
There was a problem hiding this comment.
Pull request overview
Adds an in-app setting to enable/disable Material dynamic (system) colors and persists the preference via DataStore.
Changes:
- Added a “Use System Colors” toggle to the Settings bottom sheet UI.
- Persisted the dynamic-colors preference in
TokenManagervia a new boolean DataStore key/flow. - Attempted to apply Material
DynamicColorsat app startup based on the stored preference.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/res/values/strings.xml | Adds a new string label for the dynamic-colors toggle. |
| app/src/main/res/layout/fragment_settings.xml | Introduces a MaterialSwitch for the new setting and repositions the logout button beneath it. |
| app/src/main/java/net/ardevd/tagius/features/settings/ui/SettingsBottomSheet.kt | Wires the switch to read/write the dynamic-colors preference via TokenManager. |
| app/src/main/java/net/ardevd/tagius/core/data/TokenManager.kt | Adds DataStore persistence (dynamic_colors) with a Flow<Boolean> and save method. |
| app/src/main/java/net/ardevd/tagius/MainActivity.kt | Reads the preference and conditionally applies Material dynamic colors on startup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| android:layout_marginStart="16dp" | ||
| android:layout_marginEnd="24dp" | ||
| app:layout_constraintTop_toBottomOf="@id/serverCard" | ||
| app:layout_constraintStart_toStartOf="@id/serverCard" /> |
There was a problem hiding this comment.
dynamicColorsSwitch is set to layout_width="0dp" but only has a start constraint. In ConstraintLayout, 0dp (“match constraints”) requires both start and end constraints; otherwise the view may measure to 0 width or cause layout warnings. Add an end constraint (e.g., constrain end to serverCard/parent) or change the width to wrap_content/match_parent consistently with your desired layout.
| app:layout_constraintStart_toStartOf="@id/serverCard" /> | |
| app:layout_constraintStart_toStartOf="@id/serverCard" | |
| app:layout_constraintEnd_toEndOf="@id/serverCard" /> |
| binding.dynamicColorsSwitch.isChecked = useDynamicColors | ||
| } | ||
|
|
||
| binding.dynamicColorsSwitch.setOnCheckedChangeListener { _, isChecked -> | ||
| viewLifecycleOwner.lifecycleScope.launch { | ||
| tokenManager.saveDynamicColors(isChecked) |
There was a problem hiding this comment.
The coroutine that sets dynamicColorsSwitch.isChecked can resume after the setOnCheckedChangeListener is attached, which will trigger the listener and persist a value even though it’s just initializing UI state. Consider setting the initial checked state before attaching the listener, or temporarily removing/suppressing the listener while updating isChecked.
| binding.dynamicColorsSwitch.isChecked = useDynamicColors | |
| } | |
| binding.dynamicColorsSwitch.setOnCheckedChangeListener { _, isChecked -> | |
| viewLifecycleOwner.lifecycleScope.launch { | |
| tokenManager.saveDynamicColors(isChecked) | |
| binding.dynamicColorsSwitch.setOnCheckedChangeListener(null) | |
| binding.dynamicColorsSwitch.isChecked = useDynamicColors | |
| binding.dynamicColorsSwitch.setOnCheckedChangeListener { _, isChecked -> | |
| viewLifecycleOwner.lifecycleScope.launch { | |
| tokenManager.saveDynamicColors(isChecked) | |
| } |
| val tokenManager = TokenManager(applicationContext) | ||
| val useDynamicColors = tokenManager.dynamicColorsFlow.first() | ||
| if (useDynamicColors) { | ||
| DynamicColors.applyToActivitiesIfAvailable(application) |
There was a problem hiding this comment.
DynamicColors.applyToActivitiesIfAvailable(application) registers lifecycle callbacks and is typically intended to be called from Application.onCreate() (before any Activity is created). Calling it inside MainActivity.onCreate() is likely too late for the current activity, so dynamic colors may not be applied at all in a single-activity app. Prefer applying dynamic colors in an Application subclass, or use the activity-scoped API (e.g., apply to the current activity) so this activity actually gets the dynamic theme.
| DynamicColors.applyToActivitiesIfAvailable(application) | |
| DynamicColors.applyToActivityIfAvailable(this) |
| // Dynamic color (if set) | ||
| runBlocking { | ||
| val tokenManager = TokenManager(applicationContext) | ||
| val useDynamicColors = tokenManager.dynamicColorsFlow.first() | ||
| if (useDynamicColors) { | ||
| DynamicColors.applyToActivitiesIfAvailable(application) | ||
| } | ||
| } |
There was a problem hiding this comment.
Using runBlocking on the main thread in onCreate can block UI startup and risks ANRs if DataStore I/O is slow (or if the first read is delayed). Consider a non-blocking approach (e.g., read from a synchronous source for this startup-only decision, or move the preference read + dynamic-color setup into app startup code where it won’t block the UI thread).
No description provided.